Skip to content
This repository has been archived by the owner on Oct 7, 2022. It is now read-only.

Test on all operating systems and make Miri run on CI #90

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

philippeitis
Copy link

@philippeitis philippeitis commented Nov 23, 2020

It might be a good idea to test on all operating systems, since they have differences in memory management and file operations which might cause bugs. It should run the tests in parallel, so there shouldn't be any significant overhead for this change.

This also fixes the set_env command to make Miri actually run (though I haven't fixed any bugs, so it still fails, but now it does so because it actually runs).

It might be a good idea to test on all operating systems, since they have differences in memory management and file operations which might cause bugs.
@codecov-io
Copy link

codecov-io commented Nov 23, 2020

Codecov Report

Merging #90 (2b0d67e) into master (c208619) will increase coverage by 1.48%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #90      +/-   ##
==========================================
+ Coverage   91.37%   92.85%   +1.48%     
==========================================
  Files           5        5              
  Lines         313      308       -5     
==========================================
  Hits          286      286              
+ Misses         27       22       -5     
Impacted Files Coverage Δ
src/backend/path.rs 100.00% <0.00%> (+16.12%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c208619...2b0d67e. Read the comment docs.

@philippeitis philippeitis changed the title Test on all operating systems. Test on all operating systems and fix MIRI test Nov 23, 2020
@philippeitis philippeitis changed the title Test on all operating systems and fix MIRI test Test on all operating systems and make Miri run on CI Nov 23, 2020
@TheNeikos
Copy link
Owner

I like those changes! Any ideas why windows fails?

Regarding the miri fail, this is due to a YAML dependency and is also tracked here #87

@philippeitis
Copy link
Author

philippeitis commented Nov 23, 2020

It seems to be an issue with tempfiles, since it's not possible to persist the database file due to Access is denied. This might be because taking the path and converting it to owned drops the temp file too soon, though I'm not actually certain about this (one possibly related issue?, another possibly related issue?). Manually creating a file seems to work without any problems - for instance, replacing test_path_backend_existing() with the following works, and tests the same behaviour (though, it's not as neat).

    #[test]
    #[cfg_attr(miri, ignore)]
    fn test_path_backend_existing() {
        let path = PathBuf::from("./test_path_backend_existing.db");
        {
            let file = std::fs::OpenOptions::new()
                .read(true)
                .write(true)
                .truncate(true)
                .create(true)
                .open(&path).expect("could not create temporary file");
        }
        let (mut backend, existed) = PathBackend::from_path_or_create(path.clone()).expect("could not create backend");
        assert!(existed);
        let data = [4, 5, 1, 6, 8, 1];

        backend.put_data(&data).expect("could not put data");
        assert_eq!(backend.get_data().expect("could not get data"), data);
        std::fs::remove_file(path).expect("file should be deleted");
    }

@niluxv
Copy link
Contributor

niluxv commented Dec 27, 2020

Nice. I hadn't noticed that miri did not run.

Strange windows issue. I'm afraid it will also cause issues with the integration tests.
The following already triggers the error:

fn test_file_persist() {
    let file = NamedTempFile::new().expect("could not create temporary file");
    println!("`file` path: {:?}", file.path());
    let file_path = file.path().to_owned();
    let file2 = NamedTempFile::new().expect("could not create temporary file");
    println!("`file2` path: {:?}", file2.path());
    file2.persist(file_path).expect("could not persist file2 to file");
}

Maybe the problem is that file is open (locked?) when we try to persist a second tempfile to it's path (file.path())?

@TheNeikos How about merging this and creating a tracking issue for the windows test failures?

@niluxv
Copy link
Contributor

niluxv commented Dec 27, 2020

Maybe the problem is that file is open (locked?) when we try to persist a second tempfile to it's path (file.path())?

I tested it on windows, and this is exactly the problem. I can make a PR to fix the tests. Maybe this PR can be merged first to keep the git history somewhat clean?

@niluxv
Copy link
Contributor

niluxv commented Jan 21, 2021

I went ahead and opened #92 to fix the tests on windows (and document the YAML UB).

@niluxv
Copy link
Contributor

niluxv commented Jan 8, 2022

@TheNeikos Any chance for these two PRs to be merged?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants